Rust: Distinguish path resolution expectations from type inference expectations #21282
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| * for non-methods. | ||
| */ | ||
| pragma[nomagic] | ||
| Type getAssocFunctionTypeInclNonMethodSelfAt( |
Check warning
Code scanning / CodeQL
Missing QLDoc for parameter Warning
hvitved
force-pushed
the
rust/path-resolution/type-inference-expectations
branch
from
4bd25e7 to
becd002
Compare
There was a problem hiding this comment.
Pull request overview
Updates the Rust path-resolution inline expectations tests to distinguish between symbols resolved via path resolution vs those that require type inference, making test expectations more explicit and interpretable.
Changes:
- Introduce
targetexpectations for call targets only resolvable with type inference. - Introduce
item_no_targetexpectations for items only resolvable via path resolution (and not matching the inferred call target). - Update the path-resolution inline expectations test harness to recognize and emit these additional tags.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| rust/ql/test/library-tests/path-resolution/my.rs | Updates an inline expectation to use target=to_string to reflect type-inference-only call targeting. |
| rust/ql/test/library-tests/path-resolution/main.rs | Re-labels several call-site expectations from item to target / item_no_target to reflect the new distinction. |
| rust/ql/lib/utils/test/PathResolutionInlineExpectationsTest.qll | Extends the inline expectations harness to support target and item_no_target tags in addition to item. |
paldepind
left a comment
paldepind
left a comment
There was a problem hiding this comment.
Nice. Great solution that gives the best of everything :)
| pub fn g() { | ||
| let x = MyStruct {}; // $ item=I50 | ||
| MyTrait::f(&x); // $ item=I48 | ||
| MyTrait::f(&x); // $ item_no_target=I48 target=I53 |
There was a problem hiding this comment.
Perhaps item_not_target would be clearer? no_target sounds like there is no target at all, when in fact there is one, it's just different from the item.
| fn foo() { | ||
| S3::<i32>:: // $ item=i32 | ||
| Assoc(); // $ item=S3i32AssocFunc $ SPURIOUS: item=S3boolAssocFunc (the spurious target is later filtered away by type inference) | ||
| Assoc(); // $ item=S3i32AssocFunc item_no_target=S3boolAssocFunc |
There was a problem hiding this comment.
Doesn't it still make sense to mark these as spurious? It's still clear from the annotation that the spurious result as not a target as well.
| Assoc(); // $ item=S3i32AssocFunc item_no_target=S3boolAssocFunc | |
| Assoc(); // $ item=S3i32AssocFunc SPURIOUS: item_no_target=S3boolAssocFunc |
There was a problem hiding this comment.
I think I'd prefer to keep it as-is, since we actually do expect path resolution to resolve to this item.
There was a problem hiding this comment.
Fair enough, though I think SPURIOUS can be used for things that are fully expected. For instance a query where a false positive is expected based on the implementation, but still not a desirable result. If path resolution magically didn't produce these results, we'd have fixed a spurious result right? :)
hvitved
force-pushed
the
rust/path-resolution/type-inference-expectations
branch
from
becd002 to
89e9a25
Compare
2 participants
Replaces
itemtags withtargettags when a call target can only be resolved with type inference, and replacesitemtags withitem_no_targettags when a call target can only be resolved with path resolution.This makes it clear when type inference improves upon targets resolved in path resolution.